r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

Actually I am unsure if that should be gate-test-avx_target_feature, apparently avx and avx512 are different. Please advise.

avx and avx512 are indeed different. However, I don't think that gate-test-* is the right thing to use here either way, as it's intended for testing feature gates, not to prevent the test from running if a hardware feature is unavailable. Unfortunately I don't know what the correct way to handle this is.

Actually, I'm wondering if this is even relevant anymore. This test has recently been moved to compile-pass, so it shouldn't try to execute it anymore and thus not require any particular hardware support. Looking at your logs, those builds happened when it was still a run-pass test.

On the other hand, looking at #44056 this test is supposed to test an issue that only occurs when the compiled binary is executed. Maybe it should not have been moved? /cc @RalfJung

Yeah maybe it should not have been moved, sorry. :/ Certainly won't do that again.

r? @alexcrichton

To make sure I'm getting this right, as pointed out by others the // gate-... directive doesn't actually prevent the test from running here (it's an unrelated directive) but it also looks like on master this test is no longer actually being run, it's just being compiled.

@infinity0 is the PR still needed in light of that information?

@alexcrichton

but it also looks like on master this test is no longer actually being run, it's just being compiled.

This was an oversight and should be reverted (this test has to run), so this issue will still need fixing.

@gnzlbg @alexcrichton

Yes this issue still needs fixing but the current PR is not approriate.

Indeed, looking at the issue, this test does need execution. For that we would need a way to filter tests by target features available at run-time. Shouldn't be to hard to add a flag to prevent this tests from executing (e.g. using is_x86_feature_detected!("avx") ).

I tried this, but this defeats the point of the test. The problem is that if you do something like

#![target_feature(enabled = "avx")]
fn main1() {}

fn main() { if is_x86_feature_detected!("avx") { main1() } }

no AVX instructions actually get generated and the original point of the test is defeated. The originally-problematic instructions (objdump -S $binary | grep ymm) are actually in the various panic handlers that are part of the std rust runtime. So defining specific functions as target_feature(enabled = "avx"), building the whole program by default without AVX, and conditionally calling the AVX-enabled functions at runtime is pointless, since the panic handlers still have no AVX instructions (and in fact the whole program has no AVX instructions since main1 is empty).

We basically need the test runner to conditionally run the whole program as a separate process, but ignore and only currently don't support this functionality and I'm not really familiar with the test harness to add this feature.

We do have a couple tests that use std::process to run the test as a separate process, e.g. to test that it exits with a particular signal (SEGFAULT, ...). Would that help?

I tried this, but this defeats the point of the test.

@infinity0 Oh no, that's not what I meant! What I meant is to do run-time feature detection to decide whether the test must be run at all (from the parent process). The current test runner already has a lot of logic to decide when to run each test. So maybe we can add a // only-host-target-feature-avx comment that the test runner passes, and then, if the target_arch is x86_64, and the host where the test are run supports avx (which can be checked with the is_x86_feature_detected! macro in the test runner), only then we actually run this test.

FWIW, we don't have to implement a super-general system for running tests depending on the host target features right now. That is, it is fine to just "hack" something in the test runner that handles this case properly. If we ever need to do this again, we can consider improving the test runner further.

r? @gnzlbg

Sounds like y'all have a lot more opinions/thoughts about this than I

@gnzlbg Do you think you'll get a chance to make a review happen here soon?

I think @infinity0 might have forgotten to close this. This tests was moved by mistake, so the issue this PR attempts to fix should not be happening any more.

The issue shouldn't be closed, it still exists. It's just now hidden by another bug (the incorrect move) so it won't be visible in current builds.

As of yesterday, I am still seeing this test fail when I do ./x.py test src/test/ui --test-args 44056. Let me see if I can still reproduce with current master.

@infinity0 this is not an issue, its a PR, with a fix approach that won't fix the issue. Maybe you could open a proper issue for this?

@RalfJung due to the other bug @infinity0 mentions this test shouldn't currently ever run at all. If that reproduces, we should fill a second bug for this.

Never mind, this was a case of #55537.

I've opened this issue so that we don't forget about this: #55996

We should close this PR, revert the change that moved this test, and then send a PR that improves the test runner so that this test can be skipped if the hardware doesn't support AVX.

thanks, closing in favour of further discussion on issue #55996